simplify plugin drop support #1723
Conversation
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 06m 41s |
| Commit |
|
| Committer | Scott Cytacki |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
53
|
Skipped
|
0
|
Passing
|
240
|
| View all changes introduced in this branch ↗︎ | |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1723 +/- ##
==========================================
- Coverage 85.85% 85.79% -0.06%
==========================================
Files 618 618
Lines 31831 31834 +3
Branches 8843 8231 -612
==========================================
- Hits 27327 27312 -15
- Misses 4175 4366 +191
+ Partials 329 156 -173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
this replaces the use of applyModelChange and just calls broadcastMessage directly on the tile
scytacki
force-pushed
the
188710376-simplify-di-drag
branch
from
e2eda18 to
f6a5729
Compare
There was a problem hiding this comment.
This looks good to me. I don't remember exactly why we used applyModelChange for this originally--it might have been to make all notifications consistently go through applyModelChange, or we may have just not given it much thought and went with what we'd already been doing. Either way, it's probably best to run this by Kirk before merging.
I'd also suggest removing notifyTileId from applyModelChange as a part of this work, assuming it's no longer used for anything. Otherwise, it will likely linger in the codebase and become mysterious and confusing sometime in the future.
|
I've merged this as is for now. I'll try to remove the notifyTileId infrastructure in a follow on PR. |
This replaces the use of applyModelChange and just calls broadcastMessage directly on the tile.
The reason for tackling this is that I'm trying to type the
INotifications better. Without the drag notification this type is pretty simple and consistent. So by having the drag notifications not use applyModelChange, it is now possible for applyModelChange to take a more restrictive and simple set of arguments.With that change it is easier for a simple version of applyModelChange to be included in the CODAP "library" I'm trying to create. The basic idea is that this library would support a notification system which it uses internally. The system isn't specific to sending messages to data interactives, but that is the way it will be used when the components of the library are used within the main CODAP app.
A further step could be removing the
notifyTileIdfrom the applyModelChange arguments. A tileId will not really have a meaning when the CODAP library is used outside of CODAP, but it is just a string so won't cause problems to leave it in.This is the first in a series of PRs: